Skip to content

review(workspace 2/3): gRPC handlers, compute, drivers#16

Open
rhuss wants to merge 1 commit into
mainfrom
pr-2243-part2-handlers-drivers
Open

review(workspace 2/3): gRPC handlers, compute, drivers#16
rhuss wants to merge 1 commit into
mainfrom
pr-2243-part2-handlers-drivers

Conversation

@rhuss

@rhuss rhuss commented Jul 16, 2026

Copy link
Copy Markdown
Owner

Scope: gRPC handlers (sandbox, provider, service, policy, auth), compute/SandboxIndex, Docker/Podman/Kubernetes drivers (22 files).

Split for bot review analysis. Not intended to be merged.

Summary by CodeRabbit

  • New Features
    • Added workspace management APIs for creating, viewing, listing, deleting, and managing workspace members.
    • Added workspace-aware sandbox, provider, service, credential, and SSH session operations.
    • Sandbox containers now include workspace metadata and workspace-qualified names.
  • Improvements
    • Sandbox retrieval, status checks, stopping, and deletion now reliably use sandbox IDs.
    • Added workspace isolation for resources, with support for explicitly listing across workspaces.
    • Improved Kubernetes resource naming and validation for workspace-qualified sandboxes.
  • Bug Fixes
    • Preserved workspace information when reconstructing sandboxes and processing container events.

Part 2/3 of workspace review split (PR 2243 mirror).
gRPC: workspace-scoped sandbox/provider/service/policy handlers, auth enforcement points.
Compute: workspace-aware SandboxIndex, lease scoping.
Drivers: Docker/Podman/Kubernetes workspace-qualified container names and labels.
@coderabbitai

coderabbitai Bot commented Jul 16, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

Workspace support is propagated through server persistence, sandbox/provider/policy APIs, compute drivers, and resource metadata. Docker, Podman, and Kubernetes lookup and naming now use workspace-aware identifiers, while gRPC exposes workspace management operations.

Changes

Workspace-scoped server lifecycle

Layer / File(s) Summary
Sandbox, service, session, and compute persistence
crates/openshell-server/src/compute/*, crates/openshell-server/src/grpc/sandbox.rs, crates/openshell-server/src/grpc/service.rs, crates/openshell-server/src/provider_refresh.rs
Sandbox, service endpoint, SSH session, compute lifecycle, lease, and refresh-state records now persist and query workspace metadata.
Workspace-aware provider and policy flows
crates/openshell-server/src/grpc/provider.rs, crates/openshell-server/src/grpc/policy.rs
Provider CRUD, credential resolution, policy merges, settings, profile validation, and refresh operations now accept and propagate workspace.
Workspace management RPCs
crates/openshell-server/src/grpc/mod.rs, crates/openshell-server/tests/*
Workspace creation, retrieval, listing, deletion, and membership RPC handlers are added, with test service stubs updated for the expanded trait.

Container and cluster drivers

Layer / File(s) Summary
Docker and Podman identity propagation
crates/openshell-driver-docker/src/*, crates/openshell-driver-podman/src/*
Container labels and generated names include workspace, and Podman lifecycle operations resolve containers by sandbox ID labels.
Kubernetes resource identity
crates/openshell-driver-kubernetes/src/*
Sandbox resource names combine workspace and name, metadata stores authoritative labels and annotations, and reads, deletes, existence checks, and watcher indexes use sandbox ID labels.

Estimated code review effort: 5 (Critical) | ~120 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Client
  participant SandboxRPC
  participant Store
  participant ComputeRuntime
  participant Driver
  Client->>SandboxRPC: create sandbox with workspace
  SandboxRPC->>Store: resolve workspace-scoped providers
  SandboxRPC->>ComputeRuntime: create workspace-aware sandbox
  ComputeRuntime->>Driver: provision sandbox with workspace identity
  Driver-->>ComputeRuntime: return sandbox status and workspace
  ComputeRuntime->>Store: persist sandbox lifecycle state by workspace
  SandboxRPC-->>Client: return workspace-scoped sandbox
Loading

Suggested reviewers: derekwaynecarr, drew, taylormutch

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title is clearly related to the PR and summarizes the main scope: workspace-related gRPC handlers, compute changes, and driver updates.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch pr-2243-part2-handlers-drivers

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 10

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (3)
crates/openshell-server/src/grpc/policy.rs (2)

2421-2429: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win

GetSandboxLogs resolves but discards workspace — no cross-workspace scope check.

let _workspace = super::workspace::resolve_workspace(...) is computed and dropped; the handler never verifies the target sandbox_id's workspace matches the caller's. Combined with #[rpc_auth(auth = "bearer", scope = "sandbox:read", role = "user")] (no per-sandbox ownership check) in grpc/mod.rs, any authenticated user with sandbox:read scope can read log content for a sandbox in a workspace they're not a member of. This is explicitly TODO'd as a Phase 2 item, but it's a real information-disclosure gap if this ships as-is.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/openshell-server/src/grpc/policy.rs` around lines 2421 - 2429, Update
handle_get_sandbox_logs to use the resolved workspace for authorization: load
the requested sandbox by req.sandbox_id and verify it belongs to that workspace
before fetching or returning logs, rejecting mismatches with the established
permission/not-found Status. Remove the discarded _workspace binding and
preserve the existing sandbox:read authentication requirements.

1381-1391: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win

Propagate workspace through the provider-policy helpers
crates/openshell-server/src/grpc/policy.rs:1381-1549 still has #[cfg(test)] wrappers that are called by production handlers, and both _with_catalog helpers now reference workspace without taking it. That breaks non-test builds. Remove the test-only gating and thread workspace through the _with_catalog signatures and call sites.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/openshell-server/src/grpc/policy.rs` around lines 1381 - 1391, Remove
the #[cfg(test)] gating from the provider-policy helper wrappers used by
production handlers. Thread workspace through
compute_provider_env_revision_with_catalog and the other _with_catalog helper
signatures, passing it from their wrapper and every call site so non-test builds
compile and workspace-specific behavior is preserved.
crates/openshell-server/src/grpc/provider.rs (1)

4080-4211: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win

Migrate these tests to the workspace-aware API. state_with_authoritative_profiles_over_default_grants, configure_provider_refresh_uses_authoritative_profile_sources_for_expiry_update, and delete_provider_refresh_uses_authoritative_profile_sources_for_expiry_update still use the old call shapes: the ObjectMeta/Provider literals omit workspace/profile_workspace, both refresh requests omit workspace, new_refresh_state is still called with 3 args, and get_message_by_name::<Provider> is still called with one arg.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/openshell-server/src/grpc/provider.rs` around lines 4080 - 4211,
Migrate state_with_authoritative_profiles_over_default_grants and both provider
refresh tests to the workspace-aware API: populate workspace/profile_workspace
fields in the ObjectMeta and Provider literals, include workspace in
ConfigureProviderRefreshRequest and DeleteProviderRefreshRequest, pass the
required workspace argument to new_refresh_state, and provide the workspace
argument to each get_message_by_name::<Provider> call.
🧹 Nitpick comments (2)
crates/openshell-server/src/grpc/sandbox.rs (1)

3680-3685: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Exercise the workspace-aware delete handler in this isolation test.

Direct Store::delete_by_name bypasses handle_delete_sandbox and ComputeRuntime::delete_sandbox, so the changed deletion contract is not tested. Delete through the RPC handler and verify the beta sandbox survives.

As per PR objectives, sandbox lifecycle deletion must be workspace-scoped across gRPC, compute, and persistence.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/openshell-server/src/grpc/sandbox.rs` around lines 3680 - 3685, Update
the isolation test around the “default” deletion to invoke the workspace-aware
gRPC delete path through handle_delete_sandbox, allowing it to exercise
ComputeRuntime::delete_sandbox and persistence. Replace the direct
Store::delete_by_name call, then retain assertions verifying the “beta” sandbox
with the shared name survives.
crates/openshell-server/src/grpc/mod.rs (1)

252-254: 🔒 Security & Privacy | 🔵 Trivial

Deferred workspace-authorization gaps are tracked but still open.

These TODO(phase2) markers correctly document real gaps: exec/watch/ssh/tcp-forward RPCs have no workspace field to scope the sandbox lookup, and all_workspaces on list_sandboxes/list_services/list_providers is reachable by any authenticated "user" role rather than being restricted to a Platform Admin role. Until Phase 2 lands, any authenticated user can enumerate/act across all workspaces via these paths. Worth tracking as a follow-up issue so it isn't lost before the next stack PR.

Also applies to: 271-272, 317-317, 329-329, 350-350, 375-376, 419-420

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/openshell-server/src/grpc/mod.rs` around lines 252 - 254, Track the
deferred workspace-authorization gaps from the TODO markers in the gRPC
handlers, including data-plane RPCs and all_workspaces paths. Create a follow-up
issue covering workspace scoping for sandbox actions and restricting
all-workspaces enumeration to Platform Admin users, and reference that issue in
each applicable TODO near the affected handlers.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@crates/openshell-driver-docker/src/lib.rs`:
- Around line 2925-2943: The overflow branches in the container-name
construction function can truncate away id_suffix, causing collisions. Update
the name.is_empty() and reserved >= MAX_CONTAINER_NAME_LEN paths to shorten only
the workspace/name portion while always preserving the complete id_suffix, and
add a regression test covering an oversized workspace with distinct sandbox IDs.

In `@crates/openshell-driver-kubernetes/src/driver.rs`:
- Around line 841-842: Update create_sandbox to call
validate_kube_resource_name_length with sandbox.workspace and sandbox.name
before kube_resource_name and DynamicObject::new. Propagate the validation error
with ?, ensuring oversized names return invalid_argument before contacting
Kubernetes.
- Around line 1155-1157: Update kube_resource_name to produce a collision-free
deterministic identifier for each workspace/name pair, using component escaping
or a stable hash suffix so embedded “--” values cannot alias distinct pairs.
Preserve its use as a valid Kubernetes object name and event index key, and
ensure the same input pair always returns the same result.

In `@crates/openshell-driver-podman/src/container.rs`:
- Around line 39-40: Update the Podman container discovery and filtering paths
using LABEL_SANDBOX_ID, LABEL_SANDBOX_NAME, LABEL_SANDBOX_NAMESPACE, and
LABEL_SANDBOX_WORKSPACE to also recognize the legacy openshell.sandbox-* label
keys. Continue writing only the new openshell.ai/sandbox-* labels, while
ensuring get, stop, delete, list, and watch operations remain compatible with
containers created using legacy labels.

In `@crates/openshell-driver-podman/src/driver.rs`:
- Around line 596-608: Constrain sandbox lookups to managed resources and reject
duplicate matches. In crates/openshell-driver-podman/src/driver.rs:596-608,
update find_container_name_by_id to include LABEL_MANAGED_FILTER and fail when
multiple containers match; apply the same filtering and duplicate rejection to
sandbox_exists and get_sandbox at
crates/openshell-driver-podman/src/driver.rs:677-710. Add the LABEL_MANAGED_BY
selector to get_sandbox, delete_sandbox, and sandbox_exists in
crates/openshell-driver-kubernetes/src/driver.rs:699-700, 912-913, and 994-995.

In `@crates/openshell-server/src/compute/mod.rs`:
- Line 780: Paginate every bounded store scan before processing or deleting
records: update crates/openshell-server/src/compute/mod.rs lines 780, 1080, and
1417-1422, plus crates/openshell-server/src/ssh_sessions.rs line 40, to
materialize all pages (or use stable keyset pagination) so resume,
reconciliation, workspace SSH cleanup, and expired/revoked session reaping cover
every record rather than only offset-zero results.

In `@crates/openshell-server/src/grpc/policy.rs`:
- Around line 2175-2215: Update the idempotent branch in the policy update
handler to return through update_config_response instead of constructing a bare
UpdateConfigResponse, preserving response_annotations and consistent response
behavior. Before the fallback put_policy_revision, revalidate the latest
policy/hash or use the helper’s guarded update path so a concurrent writer
cannot cause an unnecessary extra revision.

In `@crates/openshell-server/src/grpc/provider.rs`:
- Around line 1015-1027: Update dynamic_token_grant_bindings_for_provider to
return Result<Vec<DynamicTokenGrantBinding>, Status>, preserving the existing ?
propagation from get_provider_type_profile and wrapping both the empty and
successful binding results in Ok. Keep the existing call sites’ await? behavior
unchanged.

In `@crates/openshell-server/src/grpc/sandbox.rs`:
- Around line 156-157: Update validate_provider_environment_keys_unique and
validate_provider_environment_keys_unique_with_catalog to accept and forward
workspace into the workspace-scoped provider lookup, ensuring validation uses
records from the requested workspace. Add a regression test covering same-named
providers in different workspaces and verify each workspace validates against
its own records.
- Around line 138-140: Authorize access in every listed sandbox and service RPC
before reading or mutating the resolved workspace:
crates/openshell-server/src/grpc/sandbox.rs:138-140, 245-250, 269-318, 328-332,
341-342, 461-462, 554-560 and crates/openshell-server/src/grpc/service.rs:29-30,
136-137, 153-188, 205-206. After resolve_workspace, enforce caller membership
for the selected workspace, and require platform-admin authorization for each
all_workspaces branch; preserve existing RPC behavior only after these checks
succeed.

---

Outside diff comments:
In `@crates/openshell-server/src/grpc/policy.rs`:
- Around line 2421-2429: Update handle_get_sandbox_logs to use the resolved
workspace for authorization: load the requested sandbox by req.sandbox_id and
verify it belongs to that workspace before fetching or returning logs, rejecting
mismatches with the established permission/not-found Status. Remove the
discarded _workspace binding and preserve the existing sandbox:read
authentication requirements.
- Around line 1381-1391: Remove the #[cfg(test)] gating from the provider-policy
helper wrappers used by production handlers. Thread workspace through
compute_provider_env_revision_with_catalog and the other _with_catalog helper
signatures, passing it from their wrapper and every call site so non-test builds
compile and workspace-specific behavior is preserved.

In `@crates/openshell-server/src/grpc/provider.rs`:
- Around line 4080-4211: Migrate
state_with_authoritative_profiles_over_default_grants and both provider refresh
tests to the workspace-aware API: populate workspace/profile_workspace fields in
the ObjectMeta and Provider literals, include workspace in
ConfigureProviderRefreshRequest and DeleteProviderRefreshRequest, pass the
required workspace argument to new_refresh_state, and provide the workspace
argument to each get_message_by_name::<Provider> call.

---

Nitpick comments:
In `@crates/openshell-server/src/grpc/mod.rs`:
- Around line 252-254: Track the deferred workspace-authorization gaps from the
TODO markers in the gRPC handlers, including data-plane RPCs and all_workspaces
paths. Create a follow-up issue covering workspace scoping for sandbox actions
and restricting all-workspaces enumeration to Platform Admin users, and
reference that issue in each applicable TODO near the affected handlers.

In `@crates/openshell-server/src/grpc/sandbox.rs`:
- Around line 3680-3685: Update the isolation test around the “default” deletion
to invoke the workspace-aware gRPC delete path through handle_delete_sandbox,
allowing it to exercise ComputeRuntime::delete_sandbox and persistence. Replace
the direct Store::delete_by_name call, then retain assertions verifying the
“beta” sandbox with the shared name survives.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 6d6d34a1-10cf-4666-b007-6ee6cde7a9e6

📥 Commits

Reviewing files that changed from the base of the PR and between 3dee557 and 8d49b2c.

📒 Files selected for processing (22)
  • crates/openshell-driver-docker/src/lib.rs
  • crates/openshell-driver-docker/src/tests.rs
  • crates/openshell-driver-kubernetes/src/driver.rs
  • crates/openshell-driver-kubernetes/src/grpc.rs
  • crates/openshell-driver-podman/src/container.rs
  • crates/openshell-driver-podman/src/driver.rs
  • crates/openshell-driver-podman/src/grpc.rs
  • crates/openshell-driver-podman/src/watcher.rs
  • crates/openshell-server/src/auth/sandbox_methods.rs
  • crates/openshell-server/src/compute/lease.rs
  • crates/openshell-server/src/compute/mod.rs
  • crates/openshell-server/src/grpc/auth_rpc.rs
  • crates/openshell-server/src/grpc/mod.rs
  • crates/openshell-server/src/grpc/policy.rs
  • crates/openshell-server/src/grpc/provider.rs
  • crates/openshell-server/src/grpc/sandbox.rs
  • crates/openshell-server/src/grpc/service.rs
  • crates/openshell-server/src/provider_refresh.rs
  • crates/openshell-server/src/ssh_sessions.rs
  • crates/openshell-server/src/supervisor_session.rs
  • crates/openshell-server/tests/common/mod.rs
  • crates/openshell-server/tests/supervisor_relay_integration.rs

Comment on lines +2925 to 2943
// Format: openshell-{workspace}--{name}-{id}
// The workspace and id are never truncated — they ensure uniqueness.
// Only the sandbox name portion is truncated when the total exceeds
// MAX_CONTAINER_NAME_LEN.

if name.is_empty() {
let mut base = format!("{CONTAINER_NAME_PREFIX}{id_suffix}");
// The prefix is always < MAX_CONTAINER_NAME_LEN. Truncate the id
// suffix only if the sandbox id itself is pathologically long.
let mut base = format!("{CONTAINER_NAME_PREFIX}{workspace}---{id_suffix}");
if base.len() > MAX_CONTAINER_NAME_LEN {
base.truncate(MAX_CONTAINER_NAME_LEN);
}
return base;
return trim_container_name_tail(base);
}

// Reserve space for the prefix and the `-<id_suffix>` tail so the id
// suffix — which is what makes the name unique between sandboxes that
// share a human-readable prefix — is never truncated away.
let reserved = CONTAINER_NAME_PREFIX.len() + 1 + id_suffix.len();
// Reserve space for fixed parts: prefix + workspace + "--" + "-" + id
let reserved = CONTAINER_NAME_PREFIX.len() + workspace.len() + 2 + 1 + id_suffix.len();
if reserved >= MAX_CONTAINER_NAME_LEN {
// Pathological sandbox id. Fall back to `<prefix><id>` and truncate.
let mut base = format!("{CONTAINER_NAME_PREFIX}{id_suffix}");
let mut base = format!("{CONTAINER_NAME_PREFIX}{workspace}---{id_suffix}");
base.truncate(MAX_CONTAINER_NAME_LEN);
return trim_container_name_tail(base);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Inspect the relevant section with line numbers
sed -n '2900,2975p' crates/openshell-driver-docker/src/lib.rs | cat -n

# Find the helper and related constants/usages
rg -n "CONTAINER_NAME_PREFIX|MAX_CONTAINER_NAME_LEN|trim_container_name_tail|id_suffix|container name" crates/openshell-driver-docker/src/lib.rs

# Show the full helper implementation if present nearby
ast-grep outline crates/openshell-driver-docker/src/lib.rs --view expanded | sed -n '1,220p'

Repository: rhuss/OpenShell

Length of output: 15827


🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Inspect nearby tests and references to container_name_for_sandbox
rg -n "container_name_for_sandbox|MAX_CONTAINER_NAME_LEN|trim_container_name_tail|oversized|truncate" crates/openshell-driver-docker/src/lib.rs crates/openshell-driver-docker -g '*test*' -g '*.rs'

# Read the test module area around the relevant function
start=$(rg -n "^#\[cfg\(test\)\]" crates/openshell-driver-docker/src/lib.rs | head -n 1 | cut -d: -f1 || true)
if [ -n "${start:-}" ]; then
  sed -n "${start},$((start+260))p" crates/openshell-driver-docker/src/lib.rs | cat -n
fi

Repository: rhuss/OpenShell

Length of output: 16316


🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Read the container-name tests around the relevant area
sed -n '1960,2045p' crates/openshell-driver-docker/src/tests.rs | cat -n

Repository: rhuss/OpenShell

Length of output: 3528


🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Search for validation or limits on workspace/name lengths
rg -n "workspace.*len|name.*len|validate.*workspace|workspace.*validate|sandbox.*workspace|DriverSandboxTemplate|DriverSandbox" crates -g '*.rs'

# Inspect the sandbox validation area
sed -n '410,520p' crates/openshell-driver-docker/src/lib.rs | cat -n

Repository: rhuss/OpenShell

Length of output: 50371


🏁 Script executed:

python3 - <<'PY'
MAX_CONTAINER_NAME_LEN = 200
PREFIX = "openshell-"

def sanitize(value: str) -> str:
    out = []
    for ch in value:
        if ch.isascii() and (ch.isalnum() or ch in "_.-"):
            out.append(ch.lower())
        else:
            out.append("-")
    return "".join(out)

def trim_tail(value: str) -> str:
    while value and value[-1] in "-._":
        value = value[:-1]
    return value

def container_name(workspace: str, name: str, sid: str) -> str:
    id_suffix = sanitize(sid)
    workspace = sanitize(workspace)
    name = sanitize(name)

    if not name:
        base = f"{PREFIX}{workspace}---{id_suffix}"
        if len(base) > MAX_CONTAINER_NAME_LEN:
            base = base[:MAX_CONTAINER_NAME_LEN]
        return trim_tail(base)

    reserved = len(PREFIX) + len(workspace) + 2 + 1 + len(id_suffix)
    if reserved >= MAX_CONTAINER_NAME_LEN:
        base = f"{PREFIX}{workspace}---{id_suffix}"
        base = base[:MAX_CONTAINER_NAME_LEN]
        return trim_tail(base)

    budget = MAX_CONTAINER_NAME_LEN - reserved
    truncated_name = trim_tail(name[:budget]) if len(name) > budget else name
    return f"{PREFIX}{workspace}--{truncated_name}-{id_suffix}"

# Find examples where the ID suffix is truncated away.
cases = [
    ("w" * 180, "n", "id-1234567890"),
    ("w" * 190, "n", "id-1234567890"),
    ("w" * 195, "n", "id-1234567890"),
]
for ws, nm, sid in cases:
    cn = container_name(ws, nm, sid)
    print("len(workspace)=", len(ws), "len(id)=", len(sid), "len(name)=", len(cn), "endswith id?", cn.endswith(sanitize(sid)))
    print(cn)
    print()

# Search for a collision by varying only the ID while keeping a long workspace.
ws = "w" * 190
nm = "n"
a = container_name(ws, nm, "id-aaaaaaaaaaaaaaaaaaaaaaaaaaaa")
b = container_name(ws, nm, "id-bbbbbbbbbbbbbbbbbbbbbbbbbbbb")
print("collision?", a == b)
print(a)
print(b)
PY

Repository: rhuss/OpenShell

Length of output: 1375


Keep the sandbox ID when names overflow. In the name.is_empty() and reserved >= MAX_CONTAINER_NAME_LEN branches, base.truncate(MAX_CONTAINER_NAME_LEN) can cut off id_suffix entirely for long workspaces, so different sandboxes can end up with the same container name. Truncate the workspace/name portion instead and add a regression test for an oversized workspace.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/openshell-driver-docker/src/lib.rs` around lines 2925 - 2943, The
overflow branches in the container-name construction function can truncate away
id_suffix, causing collisions. Update the name.is_empty() and reserved >=
MAX_CONTAINER_NAME_LEN paths to shorten only the workspace/name portion while
always preserving the complete id_suffix, and add a regression test covering an
oversized workspace with distinct sandbox IDs.

Comment on lines +841 to +842
let kube_name = kube_resource_name(&sandbox.workspace, name);
let mut obj = DynamicObject::new(&kube_name, &agent_sandbox_api.resource);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Map the relevant file and search for validation helpers and create_sandbox call sites.
ast-grep outline crates/openshell-driver-kubernetes/src/driver.rs --view expanded || true
printf '\n--- search ---\n'
rg -n "validate_sandbox_create|validate_kube_resource_name_length|create_sandbox\(" crates/openshell-driver-kubernetes/src/driver.rs crates/openshell-driver-kubernetes/src -S
printf '\n--- file excerpt around create_sandbox ---\n'
sed -n '780,900p' crates/openshell-driver-kubernetes/src/driver.rs
printf '\n--- file excerpt around validation helpers ---\n'
rg -n "fn validate_sandbox_create|fn validate_kube_resource_name_length|kube_resource_name\(" crates/openshell-driver-kubernetes/src/driver.rs -n -S

Repository: rhuss/OpenShell

Length of output: 24893


🏁 Script executed:

#!/bin/bash
set -euo pipefail
sed -n '1,260p' crates/openshell-driver-kubernetes/src/driver.rs

Repository: rhuss/OpenShell

Length of output: 9236


🏁 Script executed:

#!/bin/bash
set -euo pipefail
grep -n "validate_sandbox_create\|validate_kube_resource_name_length\|create_sandbox(" -n crates/openshell-driver-kubernetes/src/driver.rs
sed -n '820,880p' crates/openshell-driver-kubernetes/src/driver.rs

Repository: rhuss/OpenShell

Length of output: 3428


🏁 Script executed:

#!/bin/bash
set -euo pipefail

sed -n '1,160p' crates/openshell-driver-kubernetes/src/grpc.rs
printf '\n--- callers of create_sandbox within repo ---\n'
rg -n "\.create_sandbox\(" crates/openshell-driver-kubernetes/src -S

Repository: rhuss/OpenShell

Length of output: 5643


🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Inspect how compute driver errors map to gRPC statuses.
rg -n "impl From<ComputeDriverError> for tonic::Status|enum ComputeDriverError|impl From<KubernetesDriverError> for openshell_core::ComputeDriverError" crates -S
printf '\n--- relevant core mapping ---\n'
rg -n "ComputeDriverError" crates/openshell-core -S

Repository: rhuss/OpenShell

Length of output: 1280


Validate sandbox names in create_sandbox. CreateSandbox reaches this method directly, so oversized workspace/name pairs can still hit Kubernetes and surface as an internal error instead of invalid_argument. Add validate_kube_resource_name_length(&sandbox.workspace, &sandbox.name)? before building the object.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/openshell-driver-kubernetes/src/driver.rs` around lines 841 - 842,
Update create_sandbox to call validate_kube_resource_name_length with
sandbox.workspace and sandbox.name before kube_resource_name and
DynamicObject::new. Propagate the validation error with ?, ensuring oversized
names return invalid_argument before contacting Kubernetes.

Comment on lines +1155 to +1157
fn kube_resource_name(workspace: &str, name: &str) -> String {
format!("{workspace}--{name}")
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Locate the helper and its callers.
rg -n "fn kube_resource_name|kube_resource_name\\(" crates/openshell-driver-kubernetes/src/driver.rs crates/openshell-driver-kubernetes/src -S

# Show the relevant section around the helper.
sed -n '1120,1195p' crates/openshell-driver-kubernetes/src/driver.rs

Repository: rhuss/OpenShell

Length of output: 5234


🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Inspect nearby resource-name construction for collision implications.
rg -n "format!\\(\"\\{workspace\\}--\\{name\\}\"|--" crates/openshell-driver-kubernetes/src/driver.rs -S

Repository: rhuss/OpenShell

Length of output: 2034


🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Find any Kubernetes API objects or caches keyed by this name.
rg -n "resource name|resource_name|kube_resource_name|ConfigMap|Secret|ServiceAccount|PersistentVolumeClaim|Job|Pod" crates/openshell-driver-kubernetes/src -S

Repository: rhuss/OpenShell

Length of output: 12320


🏁 Script executed:

#!/bin/bash
set -euo pipefail

sed -n '1,220p' crates/openshell-driver-kubernetes/src/driver.rs

Repository: rhuss/OpenShell

Length of output: 7862


🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Print the surrounding context of the helper and its uses.
python3 - <<'PY'
from pathlib import Path
p = Path('crates/openshell-driver-kubernetes/src/driver.rs')
text = p.read_text()
for needle in ['fn kube_resource_name', 'kube_resource_name(']:
    idx = text.find(needle)
    print(f'NEEDLE={needle} IDX={idx}')
PY

Repository: rhuss/OpenShell

Length of output: 229


🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Inspect the helper and its immediate callers.
sed -n '1140,1185p' crates/openshell-driver-kubernetes/src/driver.rs
printf '\n---\n'
rg -n "kube_resource_name\\(" crates/openshell-driver-kubernetes/src/driver.rs -n -S

Repository: rhuss/OpenShell

Length of output: 2187


🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Inspect workspace/name validation and the tests around kube_resource_name.
sed -n '5708,5765p' crates/openshell-driver-kubernetes/src/driver.rs
printf '\n---\n'
rg -n "workspace|name.*validate|validate.*workspace|DNS-1123|kube_resource_name_length" crates/openshell-driver-kubernetes/src/driver.rs -S

Repository: rhuss/OpenShell

Length of output: 10161


🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Look for other validation paths for sandbox workspace/name in the driver and shared core.
rg -n "workspace.*invalid|invalid.*workspace|sandbox\.workspace|sandbox\.name|name.*invalid|DNS-1123|validate_kube_resource_name_length|LABEL_SANDBOX_WORKSPACE|LABEL_SANDBOX_NAME" crates/openshell-driver-kubernetes/src crates/openshell-core/src -S

Repository: rhuss/OpenShell

Length of output: 5192


🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Narrowly inspect the call site that uses the generated Kubernetes name.
sed -n '820,855p' crates/openshell-driver-kubernetes/src/driver.rs

Repository: rhuss/OpenShell

Length of output: 2064


🏁 Script executed:

#!/bin/bash
set -euo pipefail

sed -n '5708,5765p' crates/openshell-driver-kubernetes/src/driver.rs
printf '\n---\n'
rg -n "workspace.*invalid|invalid.*workspace|sandbox\\.workspace|sandbox\\.name|name.*invalid|DNS-1123|validate_kube_resource_name_length|LABEL_SANDBOX_WORKSPACE|LABEL_SANDBOX_NAME" crates/openshell-driver-kubernetes/src crates/openshell-core/src -S
printf '\n---\n'
sed -n '820,855p' crates/openshell-driver-kubernetes/src/driver.rs

Repository: rhuss/OpenShell

Length of output: 9382


🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Inspect the validation helpers and the create path around sandbox identifiers.
sed -n '300,380p' crates/openshell-driver-kubernetes/src/driver.rs
printf '\n---\n'
sed -n '640,690p' crates/openshell-driver-kubernetes/src/driver.rs
printf '\n---\n'
rg -n "validate_.*workspace|validate_.*name|DNS-1123 label|DNS-1123 subdomain|sandbox\\.workspace|sandbox\\.name" crates/openshell-driver-kubernetes/src/driver.rs -n -S

Repository: rhuss/OpenShell

Length of output: 7261


🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Check whether sandbox.workspace / sandbox.name have any stricter validation in shared proto/core code.
rg -n "workspace|sandbox name|sandbox_name|sandbox\\.workspace|sandbox\\.name|DNS-1123|validate_.*sandbox" crates/openshell-core crates/openshell-driver-kubernetes -S

Repository: rhuss/OpenShell

Length of output: 27543


🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Find the protobuf/struct definitions for DriverSandbox fields and any validators on them.
rg -n "message DriverSandbox|struct DriverSandbox|workspace =|name =" crates -g '!target' -S

Repository: rhuss/OpenShell

Length of output: 50371


🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Inspect the sandbox type definition and any docs on field constraints.
rg -n "DriverSandbox|workspace|name" crates/openshell-core/src/proto -S

Repository: rhuss/OpenShell

Length of output: 153


🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Show all direct uses of the generated Kubernetes name.
rg -n "kube_resource_name\\(|DynamicObject::new\\(&kube_name|metadata\\.name|name: Some\\(\".*--.*\"\\)" crates/openshell-driver-kubernetes/src/driver.rs -S

Repository: rhuss/OpenShell

Length of output: 765


🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Inspect the sandbox create/update validators in the gateway for workspace/name character rules.
sed -n '120,260p' crates/openshell-server/src/grpc/sandbox.rs
printf '\n---\n'
sed -n '820,920p' crates/openshell-server/src/grpc/validation.rs
printf '\n---\n'
rg -n "MAX_NAME_LEN|DNS-1123|workspace.*validation|name.*validation|sandbox_name|workspace" crates/openshell-server/src/grpc/validation.rs crates/openshell-server/src/grpc/sandbox.rs -S

Repository: rhuss/OpenShell

Length of output: 21757


Make kube_resource_name collision-free. workspace--name is still ambiguous if either component can contain --, and this value is used as the Kubernetes object name and event index key. Escape the components or add a stable hash suffix so distinct sandbox pairs can’t alias each other.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/openshell-driver-kubernetes/src/driver.rs` around lines 1155 - 1157,
Update kube_resource_name to produce a collision-free deterministic identifier
for each workspace/name pair, using component escaping or a stable hash suffix
so embedded “--” values cannot alias distinct pairs. Preserve its use as a valid
Kubernetes object name and event index key, and ensure the same input pair
always returns the same result.

Comment on lines +39 to +40
pub use openshell_core::driver_utils::{
LABEL_SANDBOX_ID, LABEL_SANDBOX_NAME, LABEL_SANDBOX_NAMESPACE, LABEL_SANDBOX_WORKSPACE,

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift

Preserve read compatibility with existing Podman labels.

All discovery paths now use the new openshell.ai/sandbox-* keys. Containers created by earlier versions with openshell.sandbox-* labels become invisible to get, stop, delete, list, and watch operations. Continue writing the new keys, but retain legacy read/filter fallbacks or provide an explicit migration.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/openshell-driver-podman/src/container.rs` around lines 39 - 40, Update
the Podman container discovery and filtering paths using LABEL_SANDBOX_ID,
LABEL_SANDBOX_NAME, LABEL_SANDBOX_NAMESPACE, and LABEL_SANDBOX_WORKSPACE to also
recognize the legacy openshell.sandbox-* label keys. Continue writing only the
new openshell.ai/sandbox-* labels, while ensuring get, stop, delete, list, and
watch operations remain compatible with containers created using legacy labels.

Comment on lines +596 to +608
/// Find the Podman container name for a sandbox by its ID using label lookup.
async fn find_container_name_by_id(
&self,
sandbox_id: &str,
) -> Result<Option<String>, ComputeDriverError> {
let filter = format!("{LABEL_SANDBOX_ID}={sandbox_id}");
let entries = self
.client
.list_containers(&filter)
.await
.map_err(ComputeDriverError::from)?;
Ok(entries.first().and_then(|e| e.names.first().cloned()))
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Locate the relevant files and inspect the surrounding functions.
git ls-files 'crates/openshell-driver-podman/src/driver.rs' 'crates/openshell-driver-kubernetes/src/driver.rs'

echo '--- PODMAN outline ---'
ast-grep outline crates/openshell-driver-podman/src/driver.rs --view expanded | sed -n '1,260p'

echo '--- KUBERNETES outline ---'
ast-grep outline crates/openshell-driver-kubernetes/src/driver.rs --view expanded | sed -n '1,260p'

echo '--- PODMAN label/search snippets ---'
rg -n 'LABEL_SANDBOX_ID|managed|list_containers|inspect_container|sandbox_exists|get_sandbox|find_container_name_by_id' crates/openshell-driver-podman/src/driver.rs

echo '--- KUBERNETES label/search snippets ---'
rg -n 'LABEL_SANDBOX_ID|managed|ListParams::default\(\)\.labels|delete|exists|get_sandbox' crates/openshell-driver-kubernetes/src/driver.rs

Repository: rhuss/OpenShell

Length of output: 23659


🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Read only the relevant line ranges around the cited sites.
echo '--- PODMAN 560-730 ---'
sed -n '560,730p' crates/openshell-driver-podman/src/driver.rs | cat -n

echo '--- KUBERNETES 670-1010 ---'
sed -n '670,1010p' crates/openshell-driver-kubernetes/src/driver.rs | cat -n

Repository: rhuss/OpenShell

Length of output: 23945


🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Inspect how managed labels are defined and applied in both drivers.
echo '--- PODMAN container label definitions/usages ---'
rg -n 'LABEL_MANAGED_FILTER|LABEL_MANAGED_BY|LABEL_MANAGED_BY_VALUE|LABEL_SANDBOX_ID|labels' crates/openshell-driver-podman/src crates/openshell-driver-kubernetes/src

echo '--- PODMAN create path around labels ---'
sed -n '470,560p' crates/openshell-driver-podman/src/driver.rs | cat -n

echo '--- KUBERNETES sandbox_labels and managed-by related code ---'
sed -n '1160,1205p' crates/openshell-driver-kubernetes/src/driver.rs | cat -n

Repository: rhuss/OpenShell

Length of output: 17796


🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Read the label definitions and the Podman create path to assess ownership scoping.
echo '--- PODMAN container module ---'
sed -n '1,220p' crates/openshell-driver-podman/src/container.rs | cat -n

echo '--- PODMAN create path ---'
sed -n '439,596p' crates/openshell-driver-podman/src/driver.rs | cat -n

echo '--- KUBERNETES sandbox labels ---'
sed -n '1170,1195p' crates/openshell-driver-kubernetes/src/driver.rs | cat -n

Repository: rhuss/OpenShell

Length of output: 18422


🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Check whether Kubernetes sandbox objects carry a managed-by label and whether Podman containers use managed labels on create.
rg -n 'LABEL_MANAGED_BY_VALUE|LABEL_MANAGED_BY|sandbox_labels\(|LABEL_MANAGED_FILTER|set_labels|labels.insert|managed' crates/openshell-driver-kubernetes/src/driver.rs crates/openshell-driver-podman/src/container.rs crates/openshell-driver-podman/src/driver.rs

echo '--- KUBERNETES sandbox_labels implementation ---'
sed -n '1170,1192p' crates/openshell-driver-kubernetes/src/driver.rs | cat -n

echo '--- PODMAN container label helpers ---'
sed -n '1,220p' crates/openshell-driver-podman/src/container.rs | cat -n

Repository: rhuss/OpenShell

Length of output: 15110


Constrain sandbox-ID lookups to managed resources and fail on duplicates.
LABEL_SANDBOX_ID alone can still match the wrong object; these paths should scope to OpenShell-managed resources and avoid picking the first hit.

  • Podman find_container_name_by_id, sandbox_exists, and get_sandbox should filter on LABEL_MANAGED_FILTER and reject multiple matches.
  • Kubernetes get_sandbox, delete_sandbox, and sandbox_exists should add the LABEL_MANAGED_BY selector.
📍 Affects 2 files
  • crates/openshell-driver-podman/src/driver.rs#L596-L608 (this comment)
  • crates/openshell-driver-podman/src/driver.rs#L677-L710
  • crates/openshell-driver-kubernetes/src/driver.rs#L699-L700
  • crates/openshell-driver-kubernetes/src/driver.rs#L912-L913
  • crates/openshell-driver-kubernetes/src/driver.rs#L994-L995
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/openshell-driver-podman/src/driver.rs` around lines 596 - 608,
Constrain sandbox lookups to managed resources and reject duplicate matches. In
crates/openshell-driver-podman/src/driver.rs:596-608, update
find_container_name_by_id to include LABEL_MANAGED_FILTER and fail when multiple
containers match; apply the same filtering and duplicate rejection to
sandbox_exists and get_sandbox at
crates/openshell-driver-podman/src/driver.rs:677-710. Add the LABEL_MANAGED_BY
selector to get_sandbox, delete_sandbox, and sandbox_exists in
crates/openshell-driver-kubernetes/src/driver.rs:699-700, 912-913, and 994-995.

let records = self
.store
.list(Sandbox::object_type(), 1000, 0)
.list_by_type(Sandbox::object_type(), 1000, 0)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift

Paginate every bounded workspace/global sweep.

Each scan reads only offset zero. Records beyond the limit are never processed, and periodic sweeps repeatedly revisit the same first page.

  • crates/openshell-server/src/compute/mod.rs#L780-L780: resume all persisted sandboxes, not only the first 1,000.
  • crates/openshell-server/src/compute/mod.rs#L1080-L1080: reconcile all store records, not only the first 500.
  • crates/openshell-server/src/compute/mod.rs#L1417-L1422: scan all workspace SSH sessions before sandbox cleanup.
  • crates/openshell-server/src/ssh_sessions.rs#L40-L40: reap expired/revoked sessions beyond the first 1,000.

Materialize all pages before performing deletions, or use stable keyset pagination.

📍 Affects 2 files
  • crates/openshell-server/src/compute/mod.rs#L780-L780 (this comment)
  • crates/openshell-server/src/compute/mod.rs#L1080-L1080
  • crates/openshell-server/src/compute/mod.rs#L1417-L1422
  • crates/openshell-server/src/ssh_sessions.rs#L40-L40
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/openshell-server/src/compute/mod.rs` at line 780, Paginate every
bounded store scan before processing or deleting records: update
crates/openshell-server/src/compute/mod.rs lines 780, 1080, and 1417-1422, plus
crates/openshell-server/src/ssh_sessions.rs line 40, to materialize all pages
(or use stable keyset pagination) so resume, reconciliation, workspace SSH
cleanup, and expired/revoked session reaping cover every record rather than only
offset-zero results.

Comment on lines +2175 to +2215
let latest = state
.store
.get_latest_policy(&sandbox_id)
.await
.map_err(|e| Status::internal(format!("fetch latest policy failed: {e}")))?;

let payload = new_policy.encode_to_vec();
let hash = deterministic_policy_hash(&new_policy);

if let Some(ref current) = latest
&& current.policy_hash == hash
{
return Ok(Response::new(UpdateConfigResponse {
version: u32::try_from(current.version).unwrap_or(0),
policy_hash: hash,
settings_revision: 0,
deleted: false,
}));
}

let next_version = latest.map_or(1, |r| r.version + 1);
let policy_id = uuid::Uuid::new_v4().to_string();

state
.store
.put_policy_revision(
&policy_id,
&sandbox_id,
&workspace,
next_version,
&payload,
&hash,
)
.await
.map_err(|e| Status::internal(format!("persist policy revision failed: {e}")))?;

let _ = state
.store
.supersede_older_policies(&sandbox_id, next_version)
.await;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

file="crates/openshell-server/src/grpc/policy.rs"

# Show the surrounding function in manageable chunks
wc -l "$file"
sed -n '2050,2235p' "$file"

# Find response construction helpers and the annotations variable usage
rg -n "response_annotations|update_config_response|UpdateConfigResponse|annotations" "$file"

Repository: rhuss/OpenShell

Length of output: 13209


🏁 Script executed:

#!/bin/bash
set -euo pipefail

file="crates/openshell-server/src/grpc/policy.rs"

# Locate the handler and response helper definitions precisely
rg -n "fn .*update_config|response_annotations|update_config_response" "$file"

# Print the specific region around the handler tail
start=$(rg -n "response_annotations" "$file" | head -n 1 | cut -d: -f1)
if [ -n "${start:-}" ]; then
  s=$((start-40)); [ "$s" -lt 1 ] && s=1
  e=$((start+120))
  sed -n "${s},${e}p" "$file"
fi

Repository: rhuss/OpenShell

Length of output: 8585


Use update_config_response on the idempotent path This branch returns a bare UpdateConfigResponse, so it drops response_annotations and diverges from the rest of the handler. Reuse the helper here; the fallback write below can also emit an extra revision if another writer advances the sandbox in between.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/openshell-server/src/grpc/policy.rs` around lines 2175 - 2215, Update
the idempotent branch in the policy update handler to return through
update_config_response instead of constructing a bare UpdateConfigResponse,
preserving response_annotations and consistent response behavior. Before the
fallback put_policy_revision, revalidate the latest policy/hash or use the
helper’s guarded update path so a concurrent writer cannot cause an unnecessary
extra revision.

Comment on lines +1015 to 1027
async fn dynamic_token_grant_bindings_for_provider(
store: &Store,
provider: &Provider,
) -> Vec<DynamicTokenGrantBinding> {
let provider_name = provider.object_name().to_string();
let profile_id = normalize_provider_type(&provider.r#type).unwrap_or(provider.r#type.as_str());
let Some(profile) = get_provider_type_profile_with_catalog(catalog, profile_id) else {
return Vec::new();
let Some(profile) =
get_provider_type_profile(store, &provider.profile_workspace, profile_id).await?
else {
return Ok(Vec::new());
};
dynamic_token_grant_bindings_for_profile(&provider_name, &profile.to_proto())
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Locate the function and referenced call sites/tests.
rg -n "dynamic_token_grant_bindings_for_provider|state_with_authoritative_profiles_over_default_grants|configure_provider_refresh_uses_authoritative_profile_sources_for_expiry_update|delete_provider_refresh_uses_authoritative_profile_sources_for_expiry_update|get_message_by_name::<Provider>|new_refresh_state\\(" crates/openshell-server/src/grpc/provider.rs

# Show the relevant implementation block.
sed -n '1000,1045p' crates/openshell-server/src/grpc/provider.rs

# Show the three test regions mentioned in the comment.
sed -n '4080,4215p' crates/openshell-server/src/grpc/provider.rs

Repository: rhuss/OpenShell

Length of output: 9872


🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Inspect surrounding type signatures and uses for the provider helper.
sed -n '960,1035p' crates/openshell-server/src/grpc/provider.rs
sed -n '1935,1965p' crates/openshell-server/src/grpc/provider.rs

# Check for the request/message field shapes used in the tests.
rg -n "struct (ObjectMeta|Provider|ConfigureProviderRefreshRequest|DeleteProviderRefreshRequest)|workspace:|profile_workspace:" crates/openshell-server/src/grpc/provider.rs crates -g '!target'

Repository: rhuss/OpenShell

Length of output: 50371


🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Determine whether the provider helper's signature is used elsewhere in the file.
rg -n "dynamic_token_grant_bindings_for_provider\\(" crates/openshell-server/src/grpc/provider.rs

# Inspect request/message construction around the cited tests.
sed -n '4090,4210p' crates/openshell-server/src/grpc/provider.rs

Repository: rhuss/OpenShell

Length of output: 5100


🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Inspect the exact signatures for the helpers/types mentioned in the tests.
rg -n "fn new_refresh_state|struct NewRefreshStateConfig|trait .*get_message_by_name|fn get_message_by_name|struct ConfigureProviderRefreshRequest|struct DeleteProviderRefreshRequest|workspace:" crates/openshell-server/src crates/openshell-core/src

# Show the definitions around new_refresh_state and the request structs.
sed -n '1,260p' crates/openshell-server/src/provider_refresh.rs
sed -n '1,220p' crates/openshell-server/src/grpc/provider.rs

Repository: rhuss/OpenShell

Length of output: 50371


🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Inspect the exact signatures for the test-side helpers and request structs.
rg -n "fn new_refresh_state|struct NewRefreshStateConfig|get_message_by_name<|struct ConfigureProviderRefreshRequest|struct DeleteProviderRefreshRequest" crates/openshell-server/src crates/openshell-core/src

# Read the implementation areas around the relevant signatures.
sed -n '1,260p' crates/openshell-server/src/provider_refresh.rs
sed -n '1,180p' crates/openshell-server/src/grpc/provider.rs

Repository: rhuss/OpenShell

Length of output: 14897


Return Result from dynamic_token_grant_bindings_for_provider.

The helper uses ? on get_provider_type_profile(...).await and returns Ok(Vec::new()), so its signature must be Result<Vec<DynamicTokenGrantBinding>, Status>. Both call sites already use .await?, so the current form won’t compile.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/openshell-server/src/grpc/provider.rs` around lines 1015 - 1027,
Update dynamic_token_grant_bindings_for_provider to return
Result<Vec<DynamicTokenGrantBinding>, Status>, preserving the existing ?
propagation from get_provider_type_profile and wrapping both the empty and
successful binding results in Ok. Keep the existing call sites’ await? behavior
unchanged.

Comment on lines +138 to +140
let workspace =
super::workspace::resolve_workspace(state.store.as_ref(), &request.workspace).await?;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔒 Security & Privacy | 🔴 Critical | 🏗️ Heavy lift

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
ast-grep outline crates/openshell-server/src/grpc/workspace.rs \
  --match resolve_workspace --view expanded
rg -n -C5 'async fn (create_sandbox|get_sandbox|list_sandboxes|expose_service|get_service|list_services|delete_service)' \
  crates/openshell-server/src/grpc/mod.rs

Repository: rhuss/OpenShell

Length of output: 4992


🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Locate workspace-related grpc files
fd -a 'workspace.rs' crates/openshell-server/src || true
fd -a 'sandbox.rs' crates/openshell-server/src/grpc || true
fd -a 'service.rs' crates/openshell-server/src/grpc || true

# Map the relevant handlers and workspace resolver
ast-grep outline crates/openshell-server/src/grpc/sandbox.rs --view expanded || true
printf '\n--- service.rs ---\n'
ast-grep outline crates/openshell-server/src/grpc/service.rs --view expanded || true

# Find workspace resolution helper usage/definition
rg -n -C3 'resolve_workspace|workspace.*authorize|platform admin|Platform Admin|all_workspaces' crates/openshell-server/src/grpc

Repository: rhuss/OpenShell

Length of output: 48969


🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Find the workspace resolver and any membership/authorization helpers it uses
rg -n -C4 'async fn resolve_workspace|fn resolve_workspace|resolve_workspace\(' crates/openshell-server/src
printf '\n--- workspace-related auth helpers ---\n'
rg -n -C3 'workspace.*member|member.*workspace|authorize.*workspace|platform admin|PlatformAdmin|is_platform_admin|admin role|role = "admin"|role = "platform"' crates/openshell-server/src

Repository: rhuss/OpenShell

Length of output: 41889


🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Find the actual resolve_workspace definition and any membership checks around it.
rg -n -C6 'resolve_workspace' crates/openshell-server/src
printf '\n--- workspace member checks ---\n'
rg -n -C4 'workspace member|workspace_members|is_workspace|member.*workspace|principal.*workspace|authorized.*workspace|authorize.*membership' crates/openshell-server/src/grpc crates/openshell-server/src

Repository: rhuss/OpenShell

Length of output: 45343


🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Find the actual workspace resolver definition and workspace membership helpers.
rg -n -C5 'pub\(super\)? async fn resolve_workspace|fn resolve_workspace|resolve_workspace\(' crates openshell-core
printf '\n--- workspace membership / authorization helpers ---\n'
rg -n -C4 'workspace member|workspace_members|principal.*workspace|authorize.*membership|is_workspace_member|resolve_.*for_principal|for_principal' crates/openshell-server/src crates/openshell-core/src

Repository: rhuss/OpenShell

Length of output: 33216


🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Locate the workspace module and any resolver definition.
fd -a 'workspace*.rs' crates/openshell-server/src || true
fd -a 'workspace*' crates/openshell-server/src || true

# Search for the workspace resolver and any membership enforcement around it.
rg -n -C6 'resolve_workspace|workspace member|workspace_members|member.*workspace|principal.*workspace|authorize.*workspace|is_workspace_member' crates/openshell-server/src

Repository: rhuss/OpenShell

Length of output: 40318


🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Find the workspace module and any functions that gate access by principal/membership.
git ls-files 'crates/openshell-server/src/**' | rg '/workspace(\.rs|/|$)'
printf '\n--- module declarations ---\n'
rg -n -C3 'mod workspace|pub mod workspace|workspace::' crates/openshell-server/src/grpc/mod.rs crates/openshell-server/src/grpc/*.rs crates/openshell-server/src/*.rs
printf '\n--- resolver / membership checks ---\n'
rg -n -C5 'resolve_workspace|workspace.*member|member.*workspace|principal.*workspace|authorize.*workspace|is_workspace_member|for_principal' crates/openshell-server/src

Repository: rhuss/OpenShell

Length of output: 153


🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Locate the workspace module / resolver anywhere in openshell-server.
git ls-files 'crates/openshell-server/src/**' | rg 'workspace'
printf '\n--- possible resolver definitions ---\n'
rg -n -C5 'resolve_workspace|handle_(add|remove|list)_workspace_members|workspace member|member.*workspace|principal.*workspace|authorize.*workspace|is_workspace_member' crates/openshell-server/src

Repository: rhuss/OpenShell

Length of output: 153


🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Find the resolver and any membership/authorization logic anywhere in the repo.
printf '%s\n' '--- files named workspace ---'
fd -a 'workspace.rs' . || true
fd -a 'workspace' . || true

printf '\n%s\n' '--- resolve_workspace usages/definitions ---'
rg -n -C6 'resolve_workspace' .

printf '\n%s\n' '--- workspace access checks ---'
rg -n -C4 'workspace member|workspace_members|member.*workspace|principal.*workspace|authorize.*workspace|is_workspace_member|for_principal|membership' .

Repository: rhuss/OpenShell

Length of output: 48405


Authorize workspace access before using the requested scope.
resolve_workspace only resolves the named workspace; these handlers still need an explicit caller-membership check before reading or mutating workspace data. The all_workspaces branches also need a platform-admin gate.

  • sandbox/service RPCs: enforce workspace membership on the selected workspace.
  • all_workspaces: restrict to platform admins.
📍 Affects 2 files
  • crates/openshell-server/src/grpc/sandbox.rs#L138-L140 (this comment)
  • crates/openshell-server/src/grpc/sandbox.rs#L245-L250
  • crates/openshell-server/src/grpc/sandbox.rs#L269-L318
  • crates/openshell-server/src/grpc/sandbox.rs#L328-L332
  • crates/openshell-server/src/grpc/sandbox.rs#L341-L342
  • crates/openshell-server/src/grpc/sandbox.rs#L461-L462
  • crates/openshell-server/src/grpc/sandbox.rs#L554-L560
  • crates/openshell-server/src/grpc/service.rs#L29-L30
  • crates/openshell-server/src/grpc/service.rs#L136-L137
  • crates/openshell-server/src/grpc/service.rs#L153-L188
  • crates/openshell-server/src/grpc/service.rs#L205-L206
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/openshell-server/src/grpc/sandbox.rs` around lines 138 - 140,
Authorize access in every listed sandbox and service RPC before reading or
mutating the resolved workspace:
crates/openshell-server/src/grpc/sandbox.rs:138-140, 245-250, 269-318, 328-332,
341-342, 461-462, 554-560 and crates/openshell-server/src/grpc/service.rs:29-30,
136-137, 153-188, 205-206. After resolve_workspace, enforce caller membership
for the selected workspace, and require platform-admin authorization for each
all_workspaces branch; preserve existing RPC behavior only after these checks
succeed.

Comment on lines +156 to +157
validate_provider_environment_keys_unique(state.store.as_ref(), &workspace, &spec.providers)
.await?;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
ast-grep outline crates/openshell-server/src/grpc/provider.rs \
  --match 'validate_provider_environment_keys_unique|validate_provider_environment_keys_unique_with_catalog' \
  --view expanded
rg -n -C6 'validate_provider_environment_keys_unique' \
  crates/openshell-server/src/grpc/provider.rs

Repository: rhuss/OpenShell

Length of output: 4238


🏁 Script executed:

#!/bin/bash
set -euo pipefail

sed -n '884,980p' crates/openshell-server/src/grpc/provider.rs
printf '\n---\n'
sed -n '140,170p' crates/openshell-server/src/grpc/sandbox.rs
printf '\n---\n'
rg -n -C2 'validate_provider_environment_keys_unique_with_catalog|validate_provider_environment_keys_unique_at|validate_provider_environment_keys_unique\(' crates/openshell-server/src/grpc/provider.rs crates/openshell-server/src/grpc/sandbox.rs

Repository: rhuss/OpenShell

Length of output: 9098


Thread workspace into validate_provider_environment_keys_unique_with_catalog.
validate_provider_environment_keys_unique receives workspace, but the inner helper drops it before the workspace-scoped lookup. Same-named providers in different workspaces can be validated against the wrong records, so pass workspace through and add a cross-workspace regression test.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/openshell-server/src/grpc/sandbox.rs` around lines 156 - 157, Update
validate_provider_environment_keys_unique and
validate_provider_environment_keys_unique_with_catalog to accept and forward
workspace into the workspace-scoped provider lookup, ensuring validation uses
records from the requested workspace. Add a regression test covering same-named
providers in different workspaces and verify each workspace validates against
its own records.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant